Skip to content
This repository has been archived by the owner on Feb 13, 2023. It is now read-only.

Make drush optional #711

Merged
merged 7 commits into from
Nov 20, 2016
Merged

Make drush optional #711

merged 7 commits into from
Nov 20, 2016

Conversation

oxyc
Copy link
Collaborator

@oxyc oxyc commented Jun 6, 2016

Checking how much faster #449 would make the build. Would obviously need a lot more work. Also makefile tests will fail.

@oxyc oxyc force-pushed the drush-optional branch from 85c381b to 9fcd556 Compare June 6, 2016 23:20
@geerlingguy geerlingguy added this to the 3.2.0 milestone Jun 9, 2016
@oxyc
Copy link
Collaborator Author

oxyc commented Jun 10, 2016

I most likely won't have time to help polish out 3.2.0 and I'm guessing this will be a PR that I wont have time to finish (going offline for a few months). If anyone feels like working on this feature, feel free!

@oxyc oxyc force-pushed the drush-optional branch from 7d5debc to b7676a4 Compare June 10, 2016 02:04
@geerlingguy
Copy link
Owner

@oxyc - Godspeed! I'll likely start working on this, as I want to keep working on decoupling things inside the VM a bit more.

@geerlingguy geerlingguy modified the milestones: 3.3.0, 3.2.0 Jul 27, 2016
@geerlingguy geerlingguy modified the milestones: 3.4.0, 3.3.0 Oct 1, 2016
@oxyc
Copy link
Collaborator Author

oxyc commented Nov 15, 2016

@geerlingguy is there anything left to do here except add documentation?

@@ -194,6 +194,7 @@ installed_extras:
- drupalconsole
# - elasticsearch
# - java
# - drush
Copy link
Collaborator Author

@oxyc oxyc Nov 15, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably default to the common case where drush is installed globally, right?

Edit: And move this above elasticsearch..

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, to both.

@@ -32,6 +33,7 @@ env:
local_config: tests/ubuntu-16-postgresql.config.yml
config_dir: /var/www/drupalvm/config
TEST_INSTALLED_EXTRAS: false
DRUSH_BIN: "${DRUPALVM_DIR}/drupal/vendor/drush/drush/drush"
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With #991 drush is no longer installed in this scenario.

@@ -215,6 +216,7 @@ extra_packages:
extra_security_enabled: false

drush_version: "master"
drush_path: "{{ drupal_composer_install_dir }}/vendor/drush/drush/drush"
Copy link
Collaborator Author

@oxyc oxyc Nov 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This needs documentation for local codebases. If you're not using composer with drush as a dependency the ansible tasks will fail. The user needs to set drush_path: /usr/local/bin/drush.

Copy link
Collaborator Author

@oxyc oxyc Nov 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@geerlingguy or do you prefer if we add an ansible task to figure out the drush_path? It would work if we used the default drush_path and only override it if drush is not in installed_extras.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd rather add the task to determine the path and use default if it's not installed via Drupal VM.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My task idea didn't work. If I set the variable it's not possible to override it in config.yml. Any ideas?

@geerlingguy geerlingguy modified the milestones: 3.6.0, 3.4.0 Nov 16, 2016
- name: Update drush_path if drush is not available globally.
set_fact:
drush_path: "{{ drupal_composer_install_dir }}/vendor/drush/drush/drush"
when: "'drush' not in installed_extras"
Copy link
Collaborator Author

@oxyc oxyc Nov 16, 2016

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Due to variable precedence this will override the role default but can still be overridden in config.yml. I think? I'll test it and make sure a bit later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nevermind, it's not possible to override it.

@joestewart
Copy link
Contributor

"Always use project specific drush when available" - Thanks!

@geerlingguy
Copy link
Owner

This is looking good to me. Any other things we need to change? Since it has to be placed into installed_extras to continue behaving as it is currently, this should be a minor version bump at least.

@oxyc
Copy link
Collaborator Author

oxyc commented Nov 16, 2016

I think that's it! The only quirk with this is that you cannot override drush_path if you have drush as a composer dependency. I don't see a use case for ever wanting to do that though. If drush isn't a composer dependency, you can change the path.

@geerlingguy
Copy link
Owner

I like it a lot!

@geerlingguy geerlingguy merged commit 9018a48 into geerlingguy:master Nov 20, 2016
@geerlingguy
Copy link
Owner

(This will go into a 3.6.0 release).

@oxyc oxyc deleted the drush-optional branch April 24, 2017 15:59
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants